Skip to content

[Misc] Add 20 regression tests for 11 tool parser bug fixes#38172

Merged
chaunceyjiang merged 3 commits intovllm-project:mainfrom
bbrowning:tool-parser-regression-tests
Apr 1, 2026
Merged

[Misc] Add 20 regression tests for 11 tool parser bug fixes#38172
chaunceyjiang merged 3 commits intovllm-project:mainfrom
bbrowning:tool-parser-regression-tests

Conversation

@bbrowning
Copy link
Copy Markdown
Contributor

Purpose

Claude Code and I audited recent tool parser bug-fix PRs (Sept 2025 until now) and found that several landed without corresponding test coverage. This is purely additive test coverage to prevent regressions as we refactor, cleanup, and redesign some of these areas.

Test Plan

pytest -sv tests/tool_parsers

Test Result

All the new tests passed, and all the old ones continue to pass.

510 passed, 1 skipped, 42 xfailed, 2 warnings

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify bot added deepseek Related to DeepSeek models qwen Related to Qwen models tool-calling bug Something isn't working labels Mar 26, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive suite of regression tests across multiple tool parsers, including DeepSeekV32, GLM4-MoE, KimiK2, MinimaxM2, Mistral, Qwen3, and Step3p5. These tests address various edge cases and potential issues such as delimiter preservation, skip_special_tokens logic, handling of zero-argument and malformed tool calls, Unicode character preservation, native tool call ID extraction, anyOf nullable parameter parsing, fast detokenization, and streaming behavior with multi-parameter and variable-sized chunks. A review comment highlights a malformed JSON string in a MinimaxM2 test, which needs to be corrected to ensure the test functions as intended.

@bbrowning
Copy link
Copy Markdown
Contributor Author

Gemini got confused counting JSON braces within the xml tags within the Python strings of the test, but I double-checked the test it highlighted just to be sure. And then gave it a thumbs-down for good measure, in case they use that to improve training.

Copy link
Copy Markdown
Contributor

@sfeng33 sfeng33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the thorough work!

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 30, 2026

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @bbrowning.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Mar 30, 2026
Copy link
Copy Markdown
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stamp, given that @sfeng33 already review this.

@aarnphm
Copy link
Copy Markdown
Collaborator

aarnphm commented Mar 30, 2026

@bbrowning there is a conflict merge here. canyou fix this?

Audited recent tool parser bug-fix PRs and found that several
landed without corresponding test coverage. Added unit tests
for each fix to prevent regressions.

- Mistral: fast detokenization text detection (PR vllm-project#37209)
- Qwen3Coder: malformed XML crash, anyOf double-encoding,
  speculative decode streaming (PRs vllm-project#36774, vllm-project#36032, vllm-project#35615)
- DeepSeekV32: delimiter preservation with fast detokenization,
  skip_special_tokens adjustment (PR vllm-project#33964)
- GLM-4 MoE: zero-argument tool calls, transformers 5.x delimiter
  handling, Unicode character preservation (PRs vllm-project#32321, vllm-project#31622, vllm-project#30920)
- MiniMax M2: anyOf nullable parameter handling for non-null and
  null values (PR vllm-project#32342)
- Step3p5: MTP-style variable-chunk and multi-token streaming
  (PR vllm-project#33690)
- Kimi K2: native tool call ID extraction and multi-turn ID
  continuity (PR vllm-project#32768)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Signed-off-by: Ben Browning <bbrownin@redhat.com>
@bbrowning
Copy link
Copy Markdown
Contributor Author

Rebasing this locally, it appears two of the tests that were previously passing are now failing. I'll investigate, but it looks like between when I opened this PR and now we may have regressed on two of these test cases already...

…llm-project#38189)

After the refactor in vllm-project#38189 to use self.tools instead of request.tools,
anyOf regression tests need to provide tools at parser construction time
so the schema is available for type resolution.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Signed-off-by: Ben Browning <bbrownin@redhat.com>
@bbrowning bbrowning force-pushed the tool-parser-regression-tests branch from 6b6a09c to 386548d Compare March 31, 2026 13:47
@bbrowning
Copy link
Copy Markdown
Contributor Author

Ok, rebased and force-pushed with the conflict fix as well as adjusting the tests to move to the new format where tools are passed when constructing the parser. That is what initially caused my local failures after fixing the conflict, so it wasn't a regression in our parsers after all but just me needing to update these tests after PR 38189 landed.

@mergify mergify bot removed the needs-rebase label Mar 31, 2026
@aarnphm aarnphm added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 31, 2026
@chaunceyjiang chaunceyjiang enabled auto-merge (squash) April 1, 2026 02:16
Copy link
Copy Markdown
Collaborator

@chaunceyjiang chaunceyjiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks~

@chaunceyjiang chaunceyjiang merged commit cb0b443 into vllm-project:main Apr 1, 2026
14 checks passed
@bbrowning bbrowning deleted the tool-parser-regression-tests branch April 1, 2026 11:27
yzong-rh pushed a commit to yzong-rh/vllm that referenced this pull request Apr 3, 2026
…ject#38172)

Signed-off-by: Ben Browning <bbrownin@redhat.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
rishitdholakia13 pushed a commit to rishitdholakia13/vllm that referenced this pull request Apr 7, 2026
…ject#38172)

Signed-off-by: Ben Browning <bbrownin@redhat.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
Signed-off-by: rishitdholakia13 <rishit+github@cohere.com>
puririshi98 pushed a commit to puririshi98/vllm that referenced this pull request Apr 7, 2026
…ject#38172)

Signed-off-by: Ben Browning <bbrownin@redhat.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
Signed-off-by: Rishi Puri <riship@nvidia.com>
big-yellow-duck pushed a commit to EmbeddedLLM/vllm that referenced this pull request Apr 8, 2026
…ject#38172)

Signed-off-by: Ben Browning <bbrownin@redhat.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
mtparet pushed a commit to blackfuel-ai/vllm that referenced this pull request Apr 9, 2026
…ject#38172)

Signed-off-by: Ben Browning <bbrownin@redhat.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
EricccYang pushed a commit to EricccYang/vllm that referenced this pull request Apr 10, 2026
…ject#38172)

Signed-off-by: Ben Browning <bbrownin@redhat.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working deepseek Related to DeepSeek models qwen Related to Qwen models ready ONLY add when PR is ready to merge/full CI is needed tool-calling

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants